Expose position prop in DotTip component #14972
Conversation
Merge changes
Fix CodeEditor not loading when WordPress is installed in a subfolder…
Update the git
Merge updates
Added `position` prop to DotTip component. Closes #14923
HardeepAsrani
requested review from
aduth,
ajitbohra,
gziolo,
jorgefilipecosta,
noisysocks,
talldan and
youknowriad
as code owners
youknowriad
added
Good First Review
|
I have done some digging into this and the reasoning isn't clear so I think based on that it should be likely to work exposing. I will cc in @noisysocks also as one of the originators of a lot of Tips code. |
The rationale was that I didn't need it so I didn't add it! 😀
This has bitten me before too! 😂 |
| @@ -33,7 +34,7 @@ export function DotTip( { | |||
| return ( | |||
| <Popover | |||
| className="nux-dot-tip" | |||
| position="middle right" | |||
| position={ position || 'middle right' } | |||
There was a problem hiding this comment.
You could do this with a default argument if you want to make it more explicit that the argument is optional.
export function DotTip( {
position = 'middle right',
...
} ) {
...Totally up to you, though!
There was a problem hiding this comment.
You could do this with a default argument if you want to make it more explicit that the argument is optional.
I'd agree this would probably be the best way to communicate it as an optional prop, and it may help future maintainability if there are changes to the component which reference position in more than this one location.
There was a problem hiding this comment.
@aduth Should I make this change then?
There was a problem hiding this comment.
@aduth Should I make this change then?
It would be a worthwhile improvement if you're willing, yes.
There was a problem hiding this comment.
@aduth Made the changes. :)
|
Thanks @HardeepAsrani! |
Description
This will expose the
positionprop inDotTipcomponent which usesPopovecomponent internally to make the popover.How has this been tested?
It was tested with the default DotTip that are being used for NUX. It's worth mentioning that the positioning will only take place if space is available for it (it drove me mad because I thought it wasn't working).
Checklist:
Fixes #14923